You can also rewrite the body of this else block as:

if (!EG(exception)) {
    phongo_throw_exception_from_bson_error_t(&error TSRMLS_CC);
}

bson_free(opts);
return false;

Shouldn't we substitute the actual database name instead of "db" here? It doesn't seem to be relevant to passing our tests but it does seem proper.

It doesn't really matter with id=0, as the "ns" is only used for getMore I believe. I've changed it though.

We discussed moving server selection into Manager.c so that this function always takes a positive server_id. When that time comes, I expect this will simply append serverId to opts and we'll remove the conditional.

Just mentioning it for the record. There's no reason to make that change in this PR.

Ack

Should we call mongoc_cursor_destroy(cmd_cursor); before returning here? There could be a leak, and I don't think we have any tests that cover an error during the first advance.

On a related note, phongo_execute_query() may also have two possible leaks in the following code blocks:

if (server_id > 0 && !mongoc_cursor_set_hint(cursor, server_id)) {
	phongo_throw_exception(PHONGO_ERROR_MONGOC_FAILED TSRMLS_CC, "%s", "Could not set cursor server_id");
	/* THIS MAY LEAK */
	return false;
}

/* maxAwaitTimeMS must be set before the cursor is sent */
if (query->max_await_time_ms) {
	mongoc_cursor_set_max_await_time_ms(cursor, query->max_await_time_ms);
}

if (!phongo_advance_cursor_and_check_for_error(cursor TSRMLS_CC)) {
	/* THIS MAY LEAK */
	return false;
}

The ones for phongo_execute_query should probably go into a new PR, however, we'll probably have to think hard on how to make this fail within a test case.

I've updated it here.

I'll open a ticket for phongo_execute_query: PHPC-1043.

I might suggest "username" for these for consistency with other placeholders. I realize the placement should make it obvious, but "admin" is often used as the default authSource.

Done

I understand that this was removed because mongoc_uri_option_is_utf8() no longer reports true for "gssapiServiceName" (see: mongodb/mongo-c-driver@4e88488#diff-c1dfae86645148a1b148cd418f1a52f1). However, I'm concerned that we now might not be handling the option at all.

php_phongo_apply_options_to_uri() may need a new condition that specifically checks for an array option matching MONGOC_URI_GSSAPISERVICENAME and sets it on the libmongoc URI using the same logic in the aforementioned commit.

That's unfortunately not too easy, as the logic you point to uses mongoc_uri_parse_auth_mechanism_properties which is an internal function. I can of course copy all of it from src/libmongoc/src/mongoc/mongoc-uri.c, but that's not great. Can you think of anything better?

We do have mongoc_uri_set_mechanism_properties(), which works off of a bson_t * and is likely even easier to work with.

In php_phongo_apply_options_to_uri(), I expect you'll want to add a case for MONGOC_URI_GSSAPISERVICENAME that then creates a new BSON document (e.g. { "SERVICE_NAME": "user-supplied-value" }. For consistency with mongodb/mongo-c-driver@4e88488, you'll first want to check that there are no authMechanismProperties assigned before setting them from "gssapiServiceName". If mongoc_uri_get_mechanism_properties() returns true, then we have a warning condition (e.g. "authMechanismProperties SERVICE_NAME already set, ignoring '%s'"). Perhaps that means we do nothing, as I don't think the libmongoc warning would result in a PHP exception. Since PHPC doesn't provide any insight into libmongoc's URI struct (certainly not to PHP userland), I imagine we can't test this no-op behavior (i.e. "gssapiServiceName" won't override an "authMechanismProperties" option).

If mongoc_uri_get_mechanism_properties() returns false, we should then assert that the "gssapiServiceName" option is a string type. If not, we can continue to raise the unexpected type exceptions. Let's restore the original contents of the manager-ctor_error-003.phpt test.

If we do have a string, we can proceed with creating the BSON document from the "gssapiServiceName" value and assigning it with mongoc_uri_set_mechanism_properties().

Done

It doesn't really matter with id=0, as the "ns" is only used for getMore I believe. I've changed it though.

Ack

The ones for phongo_execute_query should probably go into a new PR, however, we'll probably have to think hard on how to make this fail within a test case.

I've updated it here.

Done

That's unfortunately not too easy, as the logic you point to uses mongoc_uri_parse_auth_mechanism_properties which is an internal function. I can of course copy all of it from src/libmongoc/src/mongoc/mongoc-uri.c, but that's not great. Can you think of anything better?

Random thought: if db contains null bytes, I assume snprintf() would simply stop appending at the character immediately preceding the first null byte. In this case, I suppose the only side effect is that we allocated more memory for ns than was needed. More importantly, this won't affect BCON_NEW() below.

libmongoc also doesn't validate the database name when executing a command (and it also appears to assume it's null-terminated when building the command parts), so I expect it'd be up to the server to report a possible error (really only if the command required a database to exist and it did not).

It doesn't matter in any case, as the value will never be used for a getMore that might need it, as the cursorID is 0. Allocating a few more bytes won't matter either, and is certainly going to be faster than checking for NULL bytes. We don't have access to the length in phongo_execute_command either although we could of course add that (for no, gain).

tmp would be a more kosher name than dummy. We don't need to insult our structs.

properties is used below without being initialized. I imagine most compilers will allocate a stack value to zero, but it's technically undefined. I'm also not sure libbson operates with a zero-initialized struct. Initializing properties to BSON_INITIALIZER seems proper.

I tend to use "dummy" if the variable isn't used at all. It's used as option 8 at http://www.dictionary.com/browse/dummy

Will fix them though :-)